Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add homework_5 #134

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add homework_5 #134

wants to merge 1 commit into from

Conversation

freefrogs
Copy link

No description provided.

end

def call
raise NotImprementedYet
siege = @warriors.present? ? @building.siege_ability = granary_ability : @building.siege_ability = 0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To sprawdzenie spokojnie by się mogło znaleźć gdzieś "wyżej", żeby nie obciążać logiki serwisu :) Serwis najlepiej jeśli ma jedną odpowiedzialność, a Twój serwis poza liczeniem ile dni przetrwa dany budynek, stwierdza upadłość budynku jeśli nie ma w nim wojowników :)

end
end

describe '#call' do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jeden describe wrapujący całość dotyczących go testów by wystarczył :)

end

def call
raise NotImprementedYet
siege = @warriors.present? ? @building.siege_ability = granary_ability : @building.siege_ability = 0
@building.save
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To zapisywanie atrybutu niepotrzebnie dociąża nam ten serwis, powinno też się znaleźć "wyżej" :) Wiąże się to też między innymi ze wzorcem dobrego projektowania aplikacji, Single Responsibility Principle. Ogólnie możnaby zoptymalizować nasz callback przez obsługę wyliczenia tych dni i zapisania tej wartości na budynku w kolejce asynchronicznej np. Sidekiq, Resque, ActiveJob :D Albo przez w ogóle zastanowieniem się czy nie wystarczy liczyć tych dni w serializerze, który by potrafił scachować wywołanie tego serwisu na jakiś czas, żeby oszczędzić pracy apce, ale to są tematy których nie było na LevelUp, nie mniej są bardzo ważne i polecam je zbadać :) napewno pomyślimy o nich w kolejnych edycjach, a napewno będą na praktykach :D

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hej,
dziękuję za komentarze, postaram się nanieść poprawki po majówce;-)
Pozdrawiam

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants